Avoid no-op EC KV reads in post-send pull sync - #900

🔧 wrench — Withdrawal always emits a marker-expiry Set-Cookie, downgrading cache privacy for all withdrawn traffic.

expire_marker has no presence guard — it appends the cookie whatever the state, including Absent. So every explicitly-withdrawn request (GPC=true, US-state opt-out) now returns Set-Cookie: ts-ec-pull-complete=; …Max-Age=0, even for the common case of a withdrawn visitor who has no EC cookie and never had a marker. On the base branch this path emitted no Set-Cookie at all, because expire_ec_cookie is gated on cookie_was_present().

Downstream that matters: apply_terminal_response_effects (crates/trusted-server-adapter-fastly/src/main.rs:418) runs enforce_set_cookie_cache_privacy after finalize, and for any cookie-bearing response that strips every edge-cache header and forces Cache-Control: private, max-age=0. template_cache_ttl (crates/trusted-server-core/src/publisher.rs:6168) also bypasses the shared template cache on Set-Cookie. Withdrawn requests never run the ad stack, so they never get with_cache_bypass — those responses were shareable before this change and are not after it, for an entire traffic class, as a side effect of a KV-read optimization.

Gating on was_present() keeps the plan's guarantee intact: it reads only the request cookie, never KV, so withdrawal still clears the marker with no KV dependency — exactly how ts-ec expiry is already gated on cookie_was_present(). explicit_withdrawal_expires_marker_without_ec_cookie still passes, since it seeds PullSyncMarkerState::Invalid (present).

Verified in an isolated worktree at this head: cargo fmt --all -- --check, cargo clippy-fastly, cargo check-axum, cargo check-cloudflare, and 332 native trusted-server-core ec:: tests all pass with this applied.

🤔 thinking — Marker issuance re-adds a Set-Cookie to responses PR #885 deliberately made cookie-free.

Same mechanism as the wrench finding above, but inherent to the design rather than a bug. The returning-user path comments that "ordinary returning-user page views no longer refresh the browser cookie" — yet a marker issue or expire makes exactly those responses cookie-bearing, so enforce_set_cookie_cache_privacy downgrades them to private, max-age=0 with edge-cache headers stripped, and Esi-mode responses stop being eligible for template-cache storage.

Bounded to roughly one response per hour per user per host, against one saved KV read per navigation, so it is plausibly a clear win — but the trade is not in the plan's risk table or in docs/guide/edge-cookies.md, and the two effects land in different budgets (KV operations vs. edge cache hit rate). Worth stating explicitly so it isn't rediscovered from a cache-hit-rate graph later.

♻️ refactor — Completeness duplicates dispatch eligibility; the two can drift.

entry_has_all_pull_partner_ids reimplements the inverse of is_partner_pull_eligible (crates/trusted-server-core/src/ec/pull_sync.rs:286-290, which is just entry.ids.get(&partner.source_domain).is_none()). They agree today only because eligibility ignores pull_sync_ttl_sec. If per-partner TTL refresh is ever wired into eligibility, completeness silently over-claims, and the marker then suppresses both the snapshot read and pull sync for up to an hour — the failure would be invisible, because both sides would still "agree" in their own tests.

Deriving one from the other makes that drift impossible:

// pull_sync.rs
pub(crate) fn is_partner_pull_eligible(partner: &PartnerConfig, kv_entry: Option<&KvEntry>) -> bool

// pull_sync_marker.rs
fn entry_has_all_pull_partner_ids(entry: &KvEntry, registry: &PartnerRegistry) -> bool {
    entry.consent.ok
        && !registry
            .pull_enabled_partners()
            .iter()
            .any(|partner| is_partner_pull_eligible(partner, Some(entry)))
}

Apply manually — can't be auto-applied as a suggestion because it changes is_partner_pull_eligible's visibility in pull_sync.rs and replaces the domain-slice parameter threaded through reconcile_marker, so it spans two files.

⛏ nitpick — The three-state loop asserts the same thing three times.

for reason in ["no partners", "complete snapshot", "unread marker state"] runs three byte-identical prepare_pull_sync_after_send(None, …) calls. None of the three named states is exercised here — all three are decided inside build_pull_sync_context, which this test never calls — so the labels read as coverage that doesn't exist. What the test actually proves (a None plan never reaches the graph factory) is worth keeping; the loop is not.

The three states themselves are already covered in core by build_pull_sync_context_skips_empty_registry_and_complete_snapshot.

Verified in an isolated worktree at this head: cargo fmt --all -- --check and cargo clippy-fastly (which compiles --all-targets, including this test) both pass with this applied.

👍 praise — Marker crypto and framing.

The details here are the ones that usually get skipped: labelled key derivation off ec.passphrase rather than using the passphrase directly as the MAC key; a NUL-delimited payload that puts the version and the fingerprint inside the MAC input, so no field can be shifted into another; constant-time verify_slice; a length cap applied before any parsing; a host-only cookie, deliberately tighter than ts-ec's Domain=.<publisher>; and Redacted plus a hand-written Debug so the value can't leak through a log line. No UID or EC ID in the cookie itself. The order-independent, set-sensitive fingerprint test covering add/remove/enable/disable is the right shape of test for this.